Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

buildDotnetModule: add support for installing pre-release tools #374663

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

buildDotnetModule: add support for installing pre-release tools #374663

wants to merge 1 commit into from
+1 −1

Conversation

gaelj
Copy link

@gaelj gaelj commented Jan 17, 2025 •
edited
Loading

Things done

Added an optional allowPreRelease argument to buildDotnetModule that will set the --prerelease flag to the dotnet tool install command.

This is because some useful Nuget packages have a tendency to stay in alpha release, so it's nice to be able to package them regardless.

We could also add the --prerelease tag unconditionally if that would be acceptable.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: dotnet Language: .NET label Jan 17, 2025
@nix-owners nix-owners bot requested a review from corngood January 17, 2025 22:47
@corngood
Copy link
Contributor

We could also add the --prerelease tag unconditionally if that would be acceptable.

I can't think of a good reason not to do this in the install. The version is pinned in the derivation.

Would specifying the version (--version ${version}) explicitly avoid the need for --prerelease?

Explicitly allowing prerelease versions could still be relevant for the update script:

# always skip prerelease versions for now
version=$(curl -fsSL "https://api.nuget.org/v3-flatcontainer/$nugetName/index.json" |
    jq -er '.versions | last(.[] | select(match("^[0-9]+\\.[0-9]+\\.[0-9]+$")))')

corngood
corngood requested changes Jan 18, 2025
View reviewed changes
Copy link
Contributor

@corngood corngood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixup commit should be squashed.

@gaelj
Copy link
Author

gaelj commented Jan 18, 2025 •
edited
Loading

Would specifying the version (--version ${version}) explicitly avoid the need for --prerelease?

No, specifying an alpha version explicitly isn't enough. The pre release flag needs to be there to install a non official version or you get an error.

I'll do some more checks. Just noticed the command errors out if we pass both version and pre release flags...

But I agree we should pass the pinned version to the DotNet tool installation command.

I'll also update the regex in the install script.

@gaelj gaelj changed the title buildDotnetModule: add support for installing pre-release tools Dratf: buildDotnetModule: add support for installing pre-release tools Jan 18, 2025
@gaelj gaelj force-pushed the nuget-allow-prerelease branch from f4a0a1e to 5edf781 Compare January 18, 2025 10:30
@gaelj gaelj changed the title Dratf: buildDotnetModule: add support for installing pre-release tools buildDotnetModule: add support for installing pre-release tools Jan 18, 2025
@gaelj gaelj force-pushed the nuget-allow-prerelease branch from 5edf781 to 4568254 Compare January 18, 2025 10:34
@gaelj
Copy link
Author

gaelj commented Jan 18, 2025

You are absolutely correct, @corngood : replacing --prerelease with --version [...] does the trick.

I modified the regex in install.sh. I tested it by running the curl command in a terminal and it works as expected. Not sure how I could test the whole script though.

All commits are now squashed.

@gaelj gaelj requested review from corngood and lostmsu January 18, 2025 11:57
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374663

x86_64-linux

✅ 10 packages built:
  • csharp-ls
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fable
  • fantomas
  • gitversion
  • pbm
  • upgrade-assistant

aarch64-linux

✅ 10 packages built:
  • csharp-ls
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fable
  • fantomas
  • gitversion
  • pbm
  • upgrade-assistant

x86_64-darwin

✅ 8 packages built:
  • csharp-ls
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fantomas
  • gitversion
  • upgrade-assistant

aarch64-darwin

✅ 7 packages built:
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fantomas
  • gitversion
  • upgrade-assistant

@gaelj gaelj force-pushed the nuget-allow-prerelease branch from 4568254 to 149eb99 Compare January 20, 2025 09:02
corngood
corngood approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@corngood corngood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@corngood corngood added the backport release-24.11 Backport PR automatically label Jan 20, 2025
@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corngood corngood corngood approved these changes

@TomaSajt TomaSajt Awaiting requested review from TomaSajt

@anpin anpin Awaiting requested review from anpin

@khaneliman khaneliman Awaiting requested review from khaneliman

@DontEatOreo DontEatOreo Awaiting requested review from DontEatOreo

@acesyde acesyde Awaiting requested review from acesyde

@mdarocha mdarocha Awaiting requested review from mdarocha

@zoriya zoriya Awaiting requested review from zoriya

@GaetanLepage GaetanLepage Awaiting requested review from GaetanLepage

@MikaelFangel MikaelFangel Awaiting requested review from MikaelFangel

@lostmsu lostmsu Awaiting requested review from lostmsu

Assignees
No one assigned
Labels
6.topic: dotnet Language: .NET 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package backport release-24.11 Backport PR automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gaelj @corngood @GaetanLepage @lostmsu @wegank